FIX: PlayerInput does not work with C# wrappers (ISXB-1535) #2188
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## develop #2188 +/- ##
===========================================
- Coverage 67.81% 67.80% -0.01%
===========================================
Files 367 367
Lines 53513 53513
===========================================
- Hits 36288 36284 -4
- Misses 17225 17229 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a bug related to PlayerInput support with C# wrappers by removing the automatic copying of action assets and adding logic to correctly disable project-wide actions when a PlayerInput is active.
- Removed asset copying logic from PlayerInput properties and Awake.
- Added handling to disable project-wide actions via InputSystem and updated corresponding tests.
- Revised tests to assert reference equality and proper enabling/disabling of action maps.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs | Removed asset copying in setter and Awake, added disabling of project-wide actions when enabled. |
| Packages/com.unity.inputsystem/InputSystem/InputSystem.cs | Introduced a flag to control enabling of actions after entering Play Mode with updated logic in OnPlayModeChange. |
| Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs | Updated and added tests to validate new behavior in asset assignment and action map states. |
| Assets/Tests/InputSystem/CoreTests_ProjectWideActions.cs | Added tests to verify that project-wide action maps are correctly disabled based on PlayerInput’s default action map. |
Comments suppressed due to low confidence (3)
Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs:342
- Removing the asset copying functionality here changes how action assets are managed. Adding a comment to explain this change will help maintainers understand why modifications to the original asset are now allowed.
var didChange = m_Actions != null;
Packages/com.unity.inputsystem/InputSystem/Plugins/PlayerInput/PlayerInput.cs:1810
- The removal of the Awake method that copied the action asset should be documented, as it significantly alters asset management and may affect users expecting a copied instance.
private void Awake()
Assets/Tests/InputSystem/Plugins/PlayerInputTests.cs:412
- Please confirm that the updated assertions correctly reflect the intended behavior, ensuring that only the appropriate action map is enabled while others remain disabled.
Assert.That(actions2.actionMaps[0].enabled, Is.False);
jfreire-unity
changed the title
jfreire-unity
force-pushed
the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
3 times, most recently
from
1b40c51 to
2eb009a
Compare
There was a problem hiding this comment.
I would suggest to test this with the ticket where the original PR was raised and now reverted.
This does not look like a solid solution in long term. This area is fragile and the fix is more a workaround. Since the optimal solution is probably not feasible to implement straight away -that's fine-, but I still wonder if there isn't an option without reverting the copy. I remember we talked about some other variants too.
| /// </remarks> | ||
| internal static bool m_EnableActionsAfterEnterPlayMode = true; | ||
|
|
||
| internal static void AllowEnableActionsAfterEnterPlayMode(bool enabled) |
There was a problem hiding this comment.
I find the naming of the method a bit odd in combination with the parameter. In this case it is hard to understand immediately if "enabled" means the actions or allowing to enable them.
I would suggest rephrasing the method name or calling the parameter something like "bool allowed".
There was a problem hiding this comment.
Thanks, I'll revisit the naming. I did forget to look at the naming again as I did a first iteration of something a bit simpler but that it didn't work.
I'll try to see if I can avoid introducing this and just move EnableActions() to a better place that doesn't require them.
There was a problem hiding this comment.
I understand this PR currently attempts to minimize changes, but....
What I struggle with regarding this property is that its basically a "kill-switch" for project-wide input actions and its questionable feature of auto-enabling. If this is disabled there is not much difference between them and a regular asset right (apart from the property accessing them statically)?
What I am thinking is this, if the problem here is that entering play-mode enables PWA actions which is desirable by some users when PlayerInput is not associated with them but not when PlayerInput is associated with them, why doesn't this dictate whether this should be done? E.g. if PlayerInput is associated with PWA, that asset may be marked. Ideally this would be much cleaner if handled on the asset level which would also phase out PWA somewhat, e.g. imagine an asset having a property "Auto-enable on entering player-mode". If we had that it would be applicable to all input action assets in the project. At the same time PlayerInput could generate an editor warning and/or console warning if you do this with an action-map marked "Auto-enable" true. Since PlayerInput tries to managed enable/disable state I think we need to accept its incompatible with PWA auto-enable setting. Currently that is hard-coded for no good reason?!
Its also not clear to me right now how this would work in a scenario where PlayerInput is used with PWA and PWA is used outside PlayerInput as well?
There was a problem hiding this comment.
At the same time, PlayerInput could generate an editor warning and/or console warning if you do this with an action-map marked "Auto-enable" true.
I think I leaning towards a solution more like this, where we let the user know in the Editor that they are using a PWA asset and that would be better to disable it to avoid problems.
And then let the users be able to disable project-wide themselves manually (which currently only works in the Player but not on Editor Play-mode).
I'm not so convinced yet of having an auto-enable feature on all assets, mostly because of its global impact. Let me think a bit more about it. In theory, it makes everything more consistent, but in practice, we get yet another combination of "settings" and more docs. I would probably be fine to confine it for PWA for now, alternatively, since it's the only one that's not following the normal rules :)
Its also not clear to me right now how this would work in a scenario where PlayerInput is used with PWA and PWA is used outside PlayerInput as well?
Are you asking why someone would use PWA directly if it's already assigned to a playerInput?
In theory, you can use PWA for UI until you enter the gameplay of a game, and once you are in gameplay, you instantiate PlayerInput which references it. Of course, there are likely more ways of achieving this. But since PWA is just a normal asset, everything that is possible with an asset is possible with PWA.
| m_Actions = value; | ||
|
|
||
| if (didChange || m_Enabled) | ||
| // copy action asset for the first player so that the original asset stays untouched | ||
| CopyActionAssetAndApplyBindingOverrides(); |
There was a problem hiding this comment.
I am still concerned with removing the copy for the first player.
In the long run this potentially leads to conflicts again as soon as action assets are accessed and operated from somewhere out of the player input.
There was a problem hiding this comment.
leads to conflicts again
Can you specify which conflicts have occurred before? I'm not aware of any other bugs/conlfits that would require a copy for the first player.
There was a problem hiding this comment.
What I am referring to is that the project wide actions was the first occurrence, since the asset can be operated freely from anywhere else without further restrictions I think it's not unlikely to encounter similar problems in the future (even if we can not directly think of one now).
There was a problem hiding this comment.
I believe project-wide actions didn't consider this use case during the design phase. In the end, PWA it's just another asset with a "small" difference that has all actions enabled. The normal behavior of an asset is to come with actions disabled. But this is not really relevant to the discussion of doing copies, which I want to explore further.
Making a copy during assignment (playerInput.asset = myAsset) is problematic for two reasons:
- It breaks expected behavior in C# - as a developer, I assume reference types maintain references rather than creating copies on assignment.
- It introduces inconsistency with our existing code, like
InputSystemUIInputModule.actionAssetwhich maintains a reference, and doesn't do a copy.
I feel we get into a slippery slope of inconsistencies if we go down the path of doing copies for every asset assignment.
I'm fine with going with a different direction, but I would like to remove the copy behavior on assignment regardless of what we agree on doing.
There was a problem hiding this comment.
I agree on that, assigning with a copy under the hood and the reference is indeed confusing.
Since this happens for all other players (except for the first), the most consistent way to do it would probably to not do it at all, or changing the behaviour to make the copy more visible.
To get a consistent behaviour I think wee need to decide for one way. What I am thinking now is:
- defining a "rootAsset" which can be assigned and then just make the player copy available. (breaking change)
- not copying the asset but outsourcing the runtime data to a metafile for instance (bigger change, but maybe not necessarily breaking change?)
I think the third option is the solution you proposed, which rolls back to the previous inconsistent state. I think that's valid with the argument to not break anything, though I believe it's not ideal in terms of consistent behaviour.
| if (m_Actions != null && m_Actions == InputSystem.actions) | ||
| { | ||
| InputSystem.AllowEnableActionsAfterEnterPlayMode(false); | ||
| InputSystem.actions.Disable(); |
There was a problem hiding this comment.
This will have an effect on the PlayerInput asset too, is disabling of all actions intended at this spot?
The bug mentioned is fixed with this PR, as well as the one this PR targets. The other solutions I previously proposed for maintaining the copy expose yet another public API to users. This exposes no other API to the users. Ideally, I would even prefer that this code would not exist, but I'm not seeing another way atm. I think if we communicate clearly that PlayerInput actions disable project-wide actions at "startup", it's still better than maintaining the copy behavior and introducing a flag to users. Project-wide actions behavior is to have all action maps enabled so that it's easier for users to get any kind of Input. |
I am not sure what you are referring to, the setter will create a copy once the PlayerInput action asset is set in runtime (which basically means set by code (which is consistent with the copy after assigning it in the component). To refer to the asset users would need to use the PlayerInput actions instead of the asset, which I think is feasible, since for multiplayer this logic would be wrong anyway- it would only affect the first user. It changes the way to access the PlayerInput asset via code, that is true, but I think the way to access the real asset for the first player, neglecting that all other players may reference to a copy is just wrong in the first place.
That might apply to the two cases, I agree. Thinking about future regressions/upcoming bugs I am not very sure if that's the best way.
Now we did not really fix one of the root causes, so my gut feeling is that there are more bugs to come.
|
|
I understand the effort to not introduce new API or change behavior significantly though, and support that. |
|
Regarding
Is my understanding correct that it essentially says that one cannot use PlayerInput and Project-wide Input Action Assets (in other places) at the same time right? Since PlayerInput acts as "manager of enabled action maps"? |
| /// <remarks> | ||
| /// This property is set to <c>true</c> by default. | ||
| /// </remarks> | ||
| internal static bool m_EnableActionsAfterEnterPlayMode = true; |
There was a problem hiding this comment.
If this is kept, it should be private.
Technically, you can. Since you can operate on both assets at runtime without restrictions. Project-wide actions asset is just a normal asset, that comes with all actions enabled by default, right? This default behavior of PWA is what clashes with PlayerInput initialization behavior since it tries to enable just a single action map (such as `PlayerInput.defaultActionMap) which is already enabled. But doesn't disable ALL the other ones. It is still possible that |
jfreire-unity
force-pushed
the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
4 times, most recently
from
2915104 to
33d8bf2
Compare
| @@ -3651,9 +3644,7 @@ internal static void OnPlayModeChange(PlayModeStateChange change) | |||
| case PlayModeStateChange.EnteredPlayMode: | |||
| s_SystemObject.enterPlayModeTime = InputRuntime.s_Instance.currentTime; | |||
| s_Manager.SyncAllDevicesAfterEnteringPlayMode(); | |||
| #if UNITY_INPUT_SYSTEM_PROJECT_WIDE_ACTIONS | |||
There was a problem hiding this comment.
Removing this for 2 reasons:
- It's inconsistent behavior with Player, which only calls it in
InitializeInPlayer() - Users could disable
InputSystem.actionsin Start, Awake, and OnEnable, and this code would be called once again. Technically, a user wouldn't be able to disable PWA in Editor PlayMode.
This reverts added changes and makes it consistent with InitializeInPlayerBehavior.
jfreire-unity
force-pushed
the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
from
c78fe53 to
306f466
Compare
|
@ritamerkl @ekcoh @Pauliusd01 I just thought it's also maybe nice to have an inspector warning for when using PlayerInput + ProjectWide actions. Would you be ok with something like this? 
|
That's a good idea! I guess it only appears if the project wide action asset is assigned? |
Yes. If another normal asset is assigned, it doesn't show. |
Sure but wording could be improved to maybe something like: "Project-Wide Actions use with Player Input is not recommended as it is a singleton reference and all action maps are enabled by default. If used, consider disabling all action maps on Start() and manually enable the ones needed" (And if there's a relevant doc link, would be nice to include as well at the end) Also, a few questions - why was single player singled out in your original? Is the only downside of using PWA in Player Input is that all maps are enabled regardless of the default map setting? Isn't that just a bug or we can't solve it due to reasons? |
Thanks for the suggestion. I'll change the text.
The underlying functionality of PWA is to have all action maps enabled by default and doesn't take into account multiple copies, resulting in potential unexpected behavior in input code for local multiplayer. It doesn't make sense to use PWA actions in PlayerInput. You can argue that it's just an asset in the end, but with all actions enabled by default that it's "easy" to use. And this doesn't go well together with PlayerInput since it decides which action maps to be enabled. That's why users need to use some manual code in case they want. |
|
I think having an inspector warning is great, run the wording with a docs representative though to get it into the right "consistent feel". |
There was a problem hiding this comment.
LGTM. Things checked: Running through existing player input test scenes and setting up player input from scratch. Also, checking the mentioned ISXB-1535 and ISXB-920 bugs. ISXB-920 does indeed regress back but the warning in the inspector should help clarify things.
One thing to note is that we're not recommending the use of a PWA assets in the player input component yet if the project has a PWA asset then it is automatically added to the player input component on creation. Which is obviously weird for us to tell them to not do x and then do it for them anyway
|
@JonMUnity I added you to make sure the warning message is consistent and makes sense. Feel free to ping me. |
|
@JonMUnity i'll merge now it and once you review it I can also change it if something is not great. |
jfreire-unity
deleted the
isxb-1535-playerinput-does-not-work-w-csharp-wrappers
branch
Description
This PR attempts to fix a PlayerInput issue when working with code generated from InputActionAssets (vulgarly called a "C# Wrapper).
In that attempt, it reverts #2144 since it introduces a regression resulting in ISXB-1535.
The fundamental problem of #2144 is caused by project-wide actions having ALL action maps enabled by default, which can create a problem if the user has the same binding in both action maps of project-wide actions assigned to
PlayerInput. In the first time a control with the same binding or more than one action map is actuated, all of its actions in different action maps will beperformed❌ See ISXB-920 for more.Also, if a user tries to call
InputSystem.actions.Disable()on Start/Awake/OnEnable in their scripts to disable action maps manually, it will not work. The actions will still be enabled withInputSystem.EnableActions()in Play-mode (this doesn't happen in Player builds).Below there's a sketch of the events code flow order in the Play-mode (last step doesn't occur in the standalone build, so the problem wouldn't exist).
flowchart LR A["InitializeInEditor(or inPlayer)<br/><i>calls EnableActions()</i>"] B["PlayerInput.OnEnable<br/><i>calls SwitchCurrentActionMaps()</i>"] C["User Scripts<br/><i>Start(), OnEnable(), Awake(), etc </i>"] D["InputSystem.OnPlayModeChange(EnteredPlayMode)<br/><i>calls EnableActions()</i>"] A --> B --> C --> D style D stroke:#ffff00,stroke-width:2pxIn #2144 a copy of the asset for a PlayerInput component is ALWAYS made, regardless if you're in single player or local multiplayer. The copied asset has all its action maps disabled by default. This allows the PlayerInput to enable the ones it needs to and fixes the problem.
However, C# wrappers have "self-contained" assets (see e.g. DefaultInputActions.asset). So if a copy of the self-contained asset is made and assigned to
playerInput.asset, using the default wrapper functionality will not work, since it acts on the self-contained asset ❌ See test introduced to make sure we fix the problem reported by the user:PlayerInput_ActionFromCodeGeneratedActionIsTheSameBeingReferenced.Instead of creating a copy of an asset when assigning it to
PlayerInput.asset, this PR proposes the simple removalInputSystem.EnableActions()during InputSystem.OnPlayModeChange(). This change makes PlayMode and Player builds enable project-wide actions at a similar point in time upon initialization.It also adds a Inspector warning for PlayerInput:
Testing status & QA
This change fixes ISXB-1535 without any changes.
However, a fix for ISXB-920 requires the user to disable project-wide actions on startup and manually enable the PlayerInput default action map in
PlayerScript.cs, such as:Added automated testing to avoid future regressions.
Overall Product Risks
Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.
Comments to reviewers
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: